Removed the change just to see if Travis is fooling me

Looks like the travis build of master has been failing since around late 2018 :(

@jlstevens is looking into it.

@jlstevens, looks like the test that's failing (for "depends on autover" tests) fails because of one or more changes to pip over time, meaning something about how the project is set up is no longer correct.

Now it's failing at downloading miniconda.

Edit: Hmm. autover is using a very old version of pyctdev, from when it was still called pyct. And pyct(dev) hardcodes the miniconda url. And the miniconda download url has changed from continuum to anaconda.

@julioasotodv The existing tests are now fixed, so you could rebase/merge/whatever. Plus a test for your change, if easy. But if you do want to add a test, note that the test framework is a bit tricky, for multiple reasons; if you can suggest a simple test to reproduce the failure you are fixing, I might be able to say how/where to add it.

Hi @ceball I believe tests are still broken, given that this commit: f97d80b

Makes the linter step in Travis complain: https://travis-ci.org/github/pyviz-dev/autover/jobs/674887606

This is showing the "bundled" example/test of autover has not been updated. We check autover works in various installation/usage scenarios, and that's one of them. So looks like you have to make changes twice...

Actually, looks like there are two "bundle examples" - so three places to update in total!

I guess the second bundle example is because at some point we discovered a problem with repository name being different from package name, or something like that.

I'm sure we could do better than requiring developers to update three files to get all the tests to pass, but it's a low traffic project...

(Sorry for the giant screenshot - on my phone!)

@ceball no, it’s fine; don’t worry. I’ll try again in a couple of minutes

Finally!

Is there a way we can easily test this change, to make sure it does not recur? I.e. how do we trigger the problem in the first place? If you were to paste:

• a code fragment that triggers the problem
• how you are using the library that triggers the problem (e.g. you installed a released package using pip? or you installed from a clone of the project's git repository via a pip editable install? or you installed via pip straight from github? or...?)
• the bad result without the fix applied - an exception, or...?
• the good result with the fix applied - just getting a __version__ string of "None" or something like that?

then I could probably add it as a test.

It would be great if you could open an issue on this repository with the above info - I've lost track of where the original report was (I'm sure there was one with more detail at some point...)!

Having said all that, I don't think it's necessary, in that I would merge the change here anyway (but @jlstevens will be the one to say what to do).

@ceball I added a new unit test in the PR.

Basically, when autover is used in a Python process created by os.fork() (just like Gunicorn does, for instance), the run_cmd function returned an erroneous exit code when used alongside asyncio. Pretty corner case, but it was driving me crazy...

Thanks for all that work! I am impressed you figured out how to modify autover's code, how to contribute to autover plus its tests, and stuck with that process through the delays etc!

Pretty corner case, but it was driving me crazy...

autover in one sentence, ha ha!

But seriously, I believe the work you have done is important. autover causes various problems in real-world environments, and if we don't eliminate them, it's at best just embarrassing for projects like param, panel, holoviews, etc ("You want me to add a package to my production environment that runs git (multiple times!) every time a user imports it? Now why would I want to add a package that does stuff like that?"...).

Without your fix, the test you added results in an exception? And with the fix, it results in the test passing - but could you add an assertion about what the value of __version__ is when it does pass? (I still don't understand what is being returned by autover for the version in this scenario, although I believe it's something like "None"? That seems wrong to me, but hugely preferable to the current situation, i.e. a crash...)

Indeed, the spawned process (for instance by Gunicorn) just chrashes with the following exception (without the fix):

File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/__init__.py", line 5, in <module>
    from . import layout # noqa
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/layout.py", line 21, in <module>
    from .io.model import hold
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/io/__init__.py", line 6, in <module>
    from .embed import embed_state # noqa
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/io/embed.py", line 21, in <module>
    from .model import add_to_doc, diff
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/io/model.py", line 14, in <module>
    from .state import state
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/io/state.py", line 14, in <module>
    from pyviz_comms import CommManager as _CommManager
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/pyviz_comms/__init__.py", line 14, in <module>
    reponame="pyviz_comms"))
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 278, in __str__
    known_stale = self._known_stale()
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 223, in _known_stale
    commit = self.commit
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 133, in commit
    return self.fetch()._commit
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 162, in fetch
    self.git_fetch(cmd)
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 212, in git_fetch
    self._update_from_vcs(output)
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 258, in _update_from_vcs
    self._release = tuple(int(el) for el in dot_split)
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 258, in <genexpr>
    self._release = tuple(int(el) for el in dot_split)
ValueError: invalid literal for int() with base 10: ''

That's why in the tests we record what the spawned process logs, and we ensure its stderr is empty.

But what is the value of __version__ going to be now with your fix? (sorry that I don't know this, but I don't know autover's code...)

It looks like you print it, but I can't find the test in the travis job logs - at least, not on my phone 😂

But what is the value of __version__ going to be now with your fix? (sorry that I don't know this, but I don't know autover's code...)

So, the issue came back from https://github.com/holoviz/param. Some other holoviz projects (such as HoloViews and Panel) make use of param.version in order to generate the version attribute for those libraries. And it turns out that param.version is an exact copy (AFAIK) of this project.

So basically the issue prevents me from using HoloViews/Panel in a async (asgi) web application, since the version generated attribute for those libraries was generated at runtime, and the runtime crashed.

Since param.version does not have any tests whatsoever, I decided to port the patch here in order to see that it does not break compatibility with the rest of param.version workflow.

As for why my test decides to print version, it is just a way to force its generation. It could very well be:

_ = autover.__version__

I'm only asking, what does it end up being set to in this scenario, after your fix? I realize it won't be the correct version, but what is it?

I'd like to put something like this in:

assert autover.__version__ is "None" # TODO this is not the correct version, but we are at least checking it does not crash; see https://github.com/pyviz-dev/autover/issues/62 for details.

I.e. I'm not asking for any more changes to the code, just to record what the situation is in the tests. Does it make sense?

Sorry for not being clear!

I now found the new test running on travis (I'm on a computer now :) ), but unfortunately the print is swallowed so I can't see for myself what __version__ is coming back as.

https://travis-ci.org/github/pyviz-dev/autover/jobs/675250008#L243

I'm only asking, what does it end up being set to in this scenario, after your fix? I realize it won't be the correct version, but what is it?

I'd like to put something like this in:

assert autover.__version__ is "None" # TODO this is not the correct version, but we are at least checking it does not crash; see https://github.com/pyviz-dev/autover/issues/62 for details.

I.e. I'm not asking for any more changes to the code, just to record what the situation is in the tests. Does it make sense?

Sorry for not being clear!

No problem, it was just to give you some context.

The problem won't be that autover.version will become None, but rather that the whole Python Process will crash. The assertion in the test checks whether that Python process outputs stuff to its stderr before crashing or not. If there's nothing in that stderr, that means that there is no crash (the goal of the patch).

However, I will try to document the test and test_asgi_server.py to make it clearer

Sorry, we are misunderstanding each other here. I am asking you to say what the value of __version__ is after your fix. I.e. now it does not crash (great!!) but presumably the reported version is not correct and is just something weird like "None"? We should record that fact, is what I am getting at.

If you also have no idea what it is after your fix, no problem. (I just assumed you would know, because you have been running it.)

I got your PR and tried it on my own machine. If I run your test without your version.py code change, the test still passes - which it presumably should not do. I assume that is happening because the test is running in a git repo, and there is actually no trouble getting the version in such a scenario - right? And instead the test needs to mimic what would happen for an installed package? Or am I wrong? (If so, maybe it's down to a difference of platform/software versions?)

Anyway, early on, I asked for a few bits of info:

Is there a way we can easily test this change, to make sure it does not recur? I.e. how do we trigger the problem in the first place? If you were to paste:

• a code fragment that triggers the problem
• how you are using the library that triggers the problem (e.g. you installed a released package using pip? or you installed from a clone of the project's git repository via a pip editable install? or you installed via pip straight from github? or...?)
• the bad result without the fix applied - an exception, or...?
• the good result with the fix applied - just getting a __version__ string of "None" or something like that?

then I could probably add it as a test.

You have supplied the first one (the code fragment) - thanks!

I still need an answer to the second one (i.e. when do you get the problem? e.g. you pip install panel from pypi, and then when you use panel in your app, you get the crash?).

The third one (what happens without your fix?), I see you previously answered in a comment on your original pyviz_comms issue (thanks!).

The fourth one I can find out for myself if you answer the second one, above.

I can then move your test to the appropriate place, so that it has the right environment to trigger the problem in the first place.

It's also possible that I could convert your test into a simpler case that will work in a unit test, but I don't want to think about that until I can first actually reproduce your problem.

Thanks!

I got your PR and tried it on my own machine. If I run your test without your version.py code change, the test still passes - which it presumably should not do. I assume that is happening because the test is running in a git repo

Exactly. The error happened either:

1. When the library comes from package without git repo (pip, conda...)
2. When git is unavailable

You can try it for instance if you temporarily add something like alias git=asdf to your ~/.bashrc, for instance, and then try to run the test.

With that said, when installed as package (with the patch), the __version__ is displayed correctly, as first the git command is issued, and if it fails (gracefully) it tries to get package info.

The changes to version.py look simple and safe to me so I am happy to merge as long as the tests go green. Right now I am seeing an error about uvloop missing so I've opened #67 to try and fix it (I would push to this PR but I don't have access to the fork).

In #67 the uvloop dependency is no longer complaining but test_run_asgi_server is failing: https://travis-ci.org/github/pyviz-dev/autover/jobs/757680271#L245

Tests are now passing in #67 after a rebase. @jbednar I am now in favor of merging that and closing this PR.

Thanks for the PR! The underlying code change has now been merged as PR #67, so closing this version. The tests here haven't been merged, but they should no longer be necessary once we merge #66, since released packages won't be calling subprocess at all at that point.